fix(button): activated outline button in toolbar no longer blends into background on MD dark mode - #29216
Merged
Conversation
…ound on dark mode
averyrousseau
changed the title
averyrousseau
marked this pull request as ready for review
March 25, 2024 19:36
thetaPC
requested changes
Mar 27, 2024
2 tasks
…ound on dark mode
averyrousseau
requested review from
liamdebeasi and
sean-perkins
as code owners
March 27, 2024 15:20
github-actions
Bot
added
package: angular
averyrousseau
marked this pull request as draft
March 27, 2024 15:20
github-actions
Bot
removed
package: angular
Contributor
|
@amandaejohnston Is this ready for review? It's marked as draft. |
Contributor
Author
|
@thetaPC No, the screenshots haven't been updated yet here. Running the job causes them to be updated off of |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Apr 1, 2024
Issue number: N/A --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> The toolbar `basic` test only checks light theme. Dark theme coverage is required to test against the bug fixed in #29216. ## What is the new behavior? <!-- Please describe the behavior or changes that are being added by this PR. --> Dark theme coverage added. This required converting the test to `page.setContent` instead of `page.goto`, so I went ahead and split the captured toolbars into multiple tests, including some cleanup of the test content. Since this included changes across many different tests, even ones not strictly related to the bug, this work has been split into a separate PR. ## Does this introduce a breaking change? - [ ] Yes - [x] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> --------- Co-authored-by: ionitron <hi@ionicframework.com>
averyrousseau
changed the title
Contributor
Author
|
Well, suddenly glad I split the test changes out the way I did, because it made me realize I regressed the styling on iOS 😆 |
This reverts commit 4d188c2.
averyrousseau
marked this pull request as ready for review
April 1, 2024 19:21
Contributor
Author
|
@thetaPC This is ready for review again 👍 |
thetaPC
approved these changes
Apr 2, 2024
thetaPC
left a comment
thetaPC
left a comment
Contributor
There was a problem hiding this comment.
LGTM, just a minor comment request
thetaPC
reviewed
Apr 2, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue number: N/A
What is the current behavior?
When using an outline-style button in a toolbar on
mddark mode, the activated style causes the button to become invisible.Steps to repro in
main:css-variablesthemes test to use the latest dark theme styles (at least for v7) by replacing the contents of this file with the styles from the docs.themes/test/css-variables/index.html.What is the new behavior?
iosmode, but had to be flipped formd, so I pulled the relevant styles into the mode-specific stylesheets.Changes to the toolbar test were split into a separate PR to keep this one clean and ensure the screenshot changes can be reviewed more effectively: #29231
Does this introduce a breaking change?
Other information